Fix my_thread_init() return value check by githubzilla · Pull Request #429 · eloqdata/eloqdoc

githubzilla · 2026-01-16T10:05:38Z

Summary by CodeRabbit

Chores
- Migrated underlying data storage infrastructure from RocksDB Cloud to EloqStore with corresponding configuration updates
- Removed legacy configuration template update mechanisms
- Transitioned storage endpoint and credential parameters to align with new backend model
- Updated internal component dependencies

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-16T10:05:57Z

Walkthrough

This PR removes legacy RocksDB Cloud storage configuration and replaces it with a new eloqstore-based model. Changes include removing the update_config_template() helper function, updating data store targets from ELOQDSS_ROCKSDB_CLOUD_S3 to ELOQDSS_ELOQSTORE, replacing rocksdb_cloud parameters with eloqstore equivalents in launch functions, and adding a new RocksDB cloud configuration file. A submodule pointer update is also included.

Changes

Cohort / File(s)	Summary
Build scripts and configuration removal `concourse/scripts/common.sh`	Removed `update_config_template()` helper function and all invocations. Replaced ELOQDSS_ROCKSDB_CLOUD_S3 with ELOQDSS_ELOQSTORE in compile_and_install_ent. Updated launch_eloqdoc and launch_eloqdoc_fast to replace rocksdb_cloud_* parameters (bucket_name, bucket_prefix, object_path, s3_endpoint_url) with eloqstore-based parameters (--eloq_store_cloud_endpoint, --eloq_store_cloud_store_path, --eloq_store_cloud_access_key, --eloq_store_cloud_secret_key, etc.).
Data substrate configurations `concourse/scripts/data_substrate.cnf`, `concourse/scripts/data_substrate.rocksdbcloud.cnf`	Removed RocksDB Cloud storage blocks from data_substrate.cnf (txlog_rocksdb_cloud_* settings, AWS credentials). Added new eloq store parameters (eloq_store_open_files_limit, eloq_store_buffer_pool_size). New file data_substrate.rocksdbcloud.cnf created with RocksDB cloud substrate configuration including cluster settings, txlog, and store sections with cloud endpoint and credential placeholders.
Submodule update `src/mongo/db/modules/eloq/data_substrate`	Updated submodule reference from a43c2d2b to 1210b035.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Run CI with ELOQDSS_ELOQSTORE #424 — Makes identical code-level changes including removal of update_config_template, data store migration from ROCKSDB_CLOUD_S3 to ELOQSTORE, and eloqstore parameter updates
build eloqstore release tarball #333 — Adds and wires up the new ELOQDSS_ELOQSTORE data store across build scripts and Concourse pipeline configurations
Update concourse config template #370 — Modifies the same concourse configuration templates and updates the data_substrate submodule pointer

Suggested labels

trigger-ci

Suggested reviewers

liunyl
yi-xmu

Poem

🐰 Scripts shed their rocky cloud,
Eloqstore steps in, proud and loud,
Config files dance in neat array,
A submodule fresh points the way! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title mentions fixing 'my_thread_init() return value check', but the actual changes involve removing helper functions, replacing RocksDB Cloud storage with EloqStore configuration, updating data substrate configuration files, and updating submodule references. The title does not reflect these primary changes.	Update the title to reflect the actual main changes, such as 'Replace RocksDB Cloud storage with EloqStore configuration' or 'Migrate data substrate configuration to EloqStore-based model'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

🧹 Recent nitpick comments

concourse/scripts/common.sh (1)
155-165: Inconsistent indentation on line 157.

Line 157 uses a tab character for indentation while the surrounding lines use spaces. This creates visual inconsistency in the code.
🔧 Suggested fix
      nohup $PREFIX/bin/eloqdoc \
            --config=./concourse/scripts/eloqdoc.yaml \
-	          --data_substrate_config=./concourse/scripts/data_substrate.cnf \
+            --data_substrate_config=./concourse/scripts/data_substrate.cnf \
            --eloq_store_cloud_endpoint=${ELOQ_AWS_S3_ENDPOINT_URL} \

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d73812b and 9dfab4b.

📒 Files selected for processing (4)

concourse/scripts/common.sh
concourse/scripts/data_substrate.cnf
concourse/scripts/data_substrate.rocksdbcloud.cnf
src/mongo/db/modules/eloq/data_substrate

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.

📚 Learning: 2025-11-28T10:55:21.020Z

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.

Applied to files:

src/mongo/db/modules/eloq/data_substrate
concourse/scripts/common.sh
concourse/scripts/data_substrate.cnf

📚 Learning: 2025-09-25T11:58:50.446Z

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.

Applied to files:

concourse/scripts/common.sh
concourse/scripts/data_substrate.cnf

📚 Learning: 2025-09-25T12:24:06.434Z

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.

Applied to files:

concourse/scripts/common.sh

📚 Learning: 2025-09-25T12:09:01.276Z

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.

Applied to files:

concourse/scripts/common.sh

📚 Learning: 2025-11-28T10:52:25.527Z

Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.

Applied to files:

concourse/scripts/common.sh

🔇 Additional comments (5)

src/mongo/db/modules/eloq/data_substrate (1)

1-1: Verify PR title alignment with submodule changes.

The PR title states "Fix my_thread_init() return value check" but the AI summary describes a RocksDB Cloud → eloqstore migration. This inconsistency needs clarification:

Does the submodule commit contain the my_thread_init() fix, the migration changes, or both?

If both are present, the PR title should reflect the primary scope of changes.

Submodule updates carry risk of hidden breaking changes or incompatibilities that are not visible in this review.

Please verify the actual contents of the data_substrate submodule update and confirm the PR title accurately reflects the changes being introduced.

concourse/scripts/data_substrate.cnf (1)

16-19: LGTM!

The new eloqstore configuration parameters are well-documented with the inline comment explaining the default value. The settings align with the migration from RocksDB Cloud to EloqStore.

concourse/scripts/data_substrate.rocksdbcloud.cnf (1)

1-29: LGTM!

This new configuration template for RocksDB Cloud backend is well-structured with appropriate placeholder credentials (CHANGEME_ACCESS_KEY_ID, CHANGEME_SECRET_KEY). The hardcoded localhost endpoint (127.0.0.1:9900) is suitable for local testing scenarios.

concourse/scripts/common.sh (2)

115-118: LGTM!

The enterprise build configuration correctly switches from ELOQDSS_ROCKSDB_CLOUD_S3 to ELOQDSS_ELOQSTORE for the data store while maintaining ROCKSDB_CLOUD_S3 for the log state. This aligns with the migration strategy where the data store moves to EloqStore but the transaction log continues using RocksDB Cloud.

178-190: LGTM!

The launch_eloqdoc_fast function mirrors the launch_eloqdoc updates consistently, with the appropriate --enable_wal=false flag for the fast path. Credentials are properly sourced from environment variables.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

githubzilla added 5 commits January 12, 2026 15:52

Update for eloqstore

ef79bd9

Fix crash

7a8920e

Update data_substrate

d7242ea

Fix launch_eloqdoc

287a184

Fix launch_eloqdoc failed

9dfab4b

githubzilla added the trigger-ci label Jan 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix my_thread_init() return value check#429

Fix my_thread_init() return value check#429
githubzilla wants to merge 5 commits intoeloqdata:mainfrom
githubzilla:ci_with_eloqstore

githubzilla commented Jan 16, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 16, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

githubzilla commented Jan 16, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

githubzilla commented Jan 16, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 16, 2026 •

edited

Loading